Implementation of the custom descriptors proposal #2295
Conversation
There was a problem hiding this comment.
I only skimmed the validation/bit-packing-RefType bits and I'll cc @fitzgen for those bits too as I'm sure he'll be interested in them when they shape up. Otherwise I left a few comments here and there. Overall thanks for this though!
| const KIND_MASK: u32 = 0b11 << 20; | ||
| const INDEX_MASK: u32 = (1 << 20) - 1; | ||
| const KIND_MASK: u32 = 0b11 << 19; | ||
| const INDEX_MASK: u32 = (1 << 19) - 1; | ||
|
|
||
| const MODULE_KIND: u32 = 0b00 << 20; | ||
| const REC_GROUP_KIND: u32 = 0b01 << 20; | ||
| const MODULE_KIND: u32 = 0b00 << 19; | ||
| const REC_GROUP_KIND: u32 = 0b01 << 19; | ||
| #[cfg(feature = "validate")] | ||
| const ID_KIND: u32 = 0b10 << 20; | ||
| const ID_KIND: u32 = 0b10 << 19; |
There was a problem hiding this comment.
For these changes mind updating the documentation on PackedIndex above? Also I think that 0b11 as a "kind" is open to represent "exact" so the index bit-space need not be shrunk?
yurydelendik
force-pushed
the
custom-descr
branch
from
07068fa to
0b11138
Compare
| } | ||
| /// Encode [`Instruction::RefCastDescNonNull`]. |
There was a problem hiding this comment.
Super nitty pick: all other methods have a newline between each other, we should match that with these new ones.
| // [ unused:u10 kind:u2 index:u20 ] | ||
| // [ unused:u11 kind:u2 index:u19 ] | ||
| // | ||
| // It must fit in 22 bits to keep `RefType` in 24 bits and `ValType` in 32 bits, | ||
| // so the top ten bits are unused. | ||
| // It must fit in 21 bits to keep `RefType` in 24 bits and `ValType` in 32 bits, | ||
| // so the top 11 bits are unused. |
There was a problem hiding this comment.
19 bits isn't enough to represent the 1_000_000 types implementation limit. That is, we would be taking on a new, more-restrictive-than-everyone-else implementation limit on the number of types with this change. (I'd expect that the can_fit_max_wasm_types_in_packed_index test would start failing).
Do we need an exact bit for all different kinds of packed indices? If not, then Alex's suggestion about using a new 0b11 kind prefix for exact references should be workable. If we need both exact type indices and exact CoreTypeIds, then I don't think it will work.
But also, this is making me think that the exact bit should be on RefType and not inside PackedIndex? PackedIndex doesn't really directly reflect a spec entity. If exactness is a property of the reftype, then the bit should be inside RefType and if it is a property of the heaptype, then the bit should be inside HeapType. So maybe after that adjustment we don't have these bitpacking issues anymore?
Draft implementation of the proposal that can be found at https://github.com/WebAssembly/custom-descriptors/blob/main/proposals/custom-descriptors/Overview.md.
Keeping it as draft:
PackedIndexorRefTypemask extension for exact type is okayThere are no tests or valid examples in the proposal yet: providing my own.